Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix lazy string format #5924

Merged
merged 2 commits into from
Dec 4, 2020

Conversation

rokonec
Copy link
Contributor

@rokonec rokonec commented Dec 1, 2020

In couple of places we have not leveraged LazyFormattedBuildEventArgs properly.
With these changes strings would not be necessary to format, in cases when logging is limited like /clp:PerformanceSummary

Related issue #2700

@rokonec
Copy link
Contributor Author

rokonec commented Dec 1, 2020

After discussion with Rainer we have decided to delete #ifdef VALIDATERESOURCESTRINGS sections as it has not been used since 2006

Copy link
Member

@benvillalobos benvillalobos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just making sure I understand this optimization at a high level.

  1. TaskLoggingHelper uses BuildMessageEventArgs to log messages, and BuildMessageEventArgs is a LazyFormattedBuildEventArgs.
  2. GetResourceString no longer auto-formats because TaskLoggingHelper ultimately uses a LazyFormattedBuildEventArgs
  3. Any calls to TaskLoggingHelper.LogMessage that used to pass in a formatted string have been modified to pass in the resource strings unformatted, thus preventing an unnecessary format

If my understanding is correct, LGTM. I double checked and there shouldn't be any other calls to LogMessage that pass in a formatted string after this PR merged.

@rokonec
Copy link
Contributor Author

rokonec commented Dec 1, 2020

@benvillalobos correct.

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

src/Framework/LazyFormattedBuildEventArgs.cs Show resolved Hide resolved
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TaskLoggingHelper is part of public API surface and - strictly technically - the way it's refactored in this PR is not 100% safe. Say that someone has a derived class:

class MyLoggingHelper : TaskLoggingHelper
{
    public override string GetResourceMessage(string resourceName)
    {
        return "Hello: " + FormatResourceString(resourceName, null);
    }
}

Now with the change in this PR calling GetResourceMessage on such a class will crash with SO exception while previously it worked perfectly fine.

Am I being paranoid? Maybe.

Here's a less contrived example: Changing the formatting behavior can currently be achieved by overriding only FormatResourceString because then you get GetResourceMessage for free. With your change it is no longer true. Actually, case in point, check out TaskLoggingHelperExtension. This class would be broken exactly like so because GetResourceMessage wouldn't get the overriden behavior.

src/Shared/TaskLoggingHelper.cs Outdated Show resolved Hide resolved
@rokonec
Copy link
Contributor Author

rokonec commented Dec 2, 2020

@ladipro This is very good catch. I will fix it.

But truth to be told, the current implementation is counter intuitive - at least to me - as one would expect FormatResourceString calls GetResourceMessage and not vice versa, but since this 'silence' contract has been published, there is not much we can do about it as there could indeed be users relying on it - and as you have pointed out, we already relay on it in TaskLoggingHelperExtension.

@rokonec rokonec requested a review from ladipro December 2, 2020 10:07
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@rokonec
Copy link
Contributor Author

rokonec commented Dec 3, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rokonec rokonec merged commit 7cc83da into dotnet:master Dec 4, 2020
@rokonec rokonec deleted the rokonec/2700-lazy-log-string-format branch December 4, 2020 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants